-
Notifications
You must be signed in to change notification settings - Fork 517
Aio connector fix workflows #2475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev/aio-connector
Are you sure you want to change the base?
Conversation
def test_ocsp_cache_when_server_is_down( | ||
mock_fetch_ocsp_response, tmpdir, random_ocsp_response_validation_cache | ||
): | ||
def test_ocsp_cache_when_server_is_down(tmpdir): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not applied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -303,7 +303,9 @@ class NotRetryableException(Exception): | |||
def fake_request_exec(**kwargs): | |||
headers = kwargs.get("headers") | |||
cnt = headers["cnt"] | |||
time.sleep(3) | |||
time.sleep( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout changes from this file are not applied to async code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!; fixed
# Test that connection respects server default when not explicitly set | ||
assert ( | ||
cnx.client_session_keep_alive == server_default | ||
), f"Expected client_session_keep_alive={server_default} (server default), got {cnx.client_session_keep_alive}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can have those clear messages in async version as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
e673724
to
0dc54a6
Compare
149e337
to
322852d
Compare
assert ret == {"success": True, "data": "valid data"} | ||
assert not rest._connection.errorhandler.called # no error | ||
|
||
# first attempt to reach timeout even if the exception is retryable | ||
cnt.reset() | ||
ret = rest.fetch(timeout=1, **default_parameters) | ||
ret = rest.fetch( | ||
timeout=0.001, **default_parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not applied to the async code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
assert ret == {} | ||
assert rest._connection.errorhandler.called # error | ||
|
||
# not retryable excpetion | ||
cnt.set(NOT_RETRYABLE) | ||
with pytest.raises(NotRetryableException): | ||
rest.fetch(timeout=7, **default_parameters) | ||
rest.fetch(timeout=5, **default_parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not applied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
host=db_parameters["sf_host"], | ||
port=db_parameters["sf_port"], | ||
) | ||
with conn_cnx("admin") as admin_cnxn: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be missed in async implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
assert ret[1] == "true" | ||
|
||
# Test client_session_keep_alive=False (connection parameter) | ||
with conn_cnx(client_session_keep_alive=False) as connection: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed from ..alive=None to ..alive=False
and the expected behavior was changed, since in the last assert we expect now "false", not "true" as it is in asyncio
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
@@ -69,21 +69,9 @@ def test_transaction(conn_cnx, db_parameters): | |||
assert total == 13824, "total integer" | |||
|
|||
|
|||
def test_connection_context_manager(request, db_parameters): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be missed in the async implementation as well as the changes below in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, as well as unlocked the test - it was marked as "xfail" due to "await" inside
Cherry-picks required to fix pipelines